-
Notifications
You must be signed in to change notification settings - Fork 93
Bump to Java 11 #150
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bump to Java 11 #150
Conversation
`graphql-java` is already on Java 11 so we take the liberty of bumping this library to the same version to unlock some nice features.
identityLoader.dispatch(); | ||
|
||
await().until(() -> future1.isDone() && future2.isDone()); | ||
assertThat(loadCalls, equalTo(singletonList(singletonList(key1)))); | ||
assertThat(loadCalls.size(), equalTo(1)); | ||
assertThat(future1.get(), equalTo(key1)); | ||
assertThat(future2.get(), equalTo(key1)); | ||
assertThat(future2.get(), equalTo(key2)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry - why is this test changed for Java 11 ? was it wrong? and if so - why was it passing ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A brief inspection indicates the test shouldn't have been working with Java 8. We can see this because the identityLoader
doesn't have identical types for its K
and V
type parameters - if we pass in a JsonObject
to a loader generated from idLoader
, we should expect to receive that same JsonObject
coming out, but that's not what this test was checking.
As to why it was passing at all - I suspect it has something to do with this bug that was fixed with Java 9. Without digging into the javac
-generated bytecode, it looks like a class check was missing from bytecode generated with Java 8. As such, when we tried to run with this code with Java 11, we started failing because we tried to cast a JsonObject
(the returned value) to an Integer
(the V
in the DataLoader
).
As to why I changed it from key1
to key2
- while key1.equals(key2)
is true, it seemed neater to check we got key2
for the future (as it corresponded to future2
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I just looked into it - <K,V> are all just Object at runtime and since we never treated a value as a JsonObject - well the test got away with it. Makes sense to fix
Wow thanks for upgrading to Java 11 @AlexandreCarlton ! |
graphql-java
is already on Java 11 so we take the liberty of bumping this library to the same version to unlock some nice features.